Add support for eth68 #4866

deffrian · 2022-11-05T12:34:12Z

Fixes | Closes | Resolves #

Changes:

NewPooledTransactionHashesMessage now has 3 parameters: hashes, transaction sizes and types.

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply

Bugfix (non-breaking change which fixes an issue)
[ x ] New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation Update
Code style update (formatting, renaming)
Refactoring (no functional changes, no api changes)
Build related changes
Other (please describe):

Testing

Requires testing

[ x ] Yes
No

In case you checked yes, did you write tests??

[ x ] Yes
No

Comments about testing , should you have some (optional)
We should test it against geth, as they have eth68 merged

marcindsobczak

Looks correct, now we need to test it with Geth nodes supporting eth/68

src/Nethermind/Nethermind.Network/P2P/Subprotocols/Eth/V68/Eth68ProtocolHandler.cs

...d.Network/P2P/Subprotocols/Eth/V68/Messages/NewPooledTransactionHashesMessageSerializer68.cs

marcindsobczak · 2022-11-11T12:16:27Z

Code looks good to me. How was it tested with Geth? Which scenarios we have covered? Do you consider it as ready to merge?

deffrian · 2022-11-11T12:26:45Z

Code looks good to me. How was it tested with Geth? Which scenarios we have covered? Do you consider it as ready to merge?

I removed all eth protocols except eth68. Started new geth node with current master and connected them together.
1). To check that we receive transactions I just checked txPool via txpool_content.
2). To check that we can send transactions I've sent transactions through Nethermind using Metamask.

It looks like those two nodes are the only eth68-compatible nodes in sepolia network. Btw I can share geth node address and you can double-check everything, should be pretty fast

I think it's ready to be merged.

marcindsobczak

I runned eth protocol hive tests, looks like we have some regression:
https://github.com/NethermindEth/nethermind/actions/runs/3445494354/jobs/5749273330

With TestBlockHashAnnounce and TestOldAnnounce we had problems before, but failing TestTransaction and TestLargeTxRequest is a regression

"14":{"name":"TestTransaction (nethermind)","description":"","start":"2022-11-11T13:55:19.464384531Z","end":"2022-11-11T13:55:19.465879952Z","summaryResult":{"pass":false,"details":" Testing tx propagation 0: sending tx 0x7d58c33ec2867fd7f9631b31258468d8b1b9cf71f57672e12eacc4e5a63ed84a 1000000 134172\n send successful tx test failed: unexpected message in sendSuccessfulTx: (*ethtest.Error)(could not rlp decode message: rlp: value size exceeds available input length)\n \n"},"clientInfo":null},

"16":{"name":"TestLargeTxRequest (nethermind)","description":"","start":"2022-11-11T13:55:19.491286912Z","end":"2022-11-11T13:55:19.492174225Z","summaryResult":{"pass":false,"details":" failed to send next block: failed to announce block: unexpected: (*ethtest.Error)(could not rlp decode message: rlp: value size exceeds available input length)\n \n"},"clientInfo":null},

Looks like there is still something wrong with rlp

marcindsobczak · 2022-11-11T15:05:15Z

That's strange

2022-11-11 13:55:19.4511|Error in communication with [Node|     172.17.0.4:35760|eth68|]: System.ArgumentException: Value does not fall within the expected range.
   at Nethermind.Serialization.Rlp.RlpStream.DecodeUInt256() in /src/Nethermind/Nethermind.Serialization.Rlp/RlpStream.cs:line 743
   at Nethermind.Serialization.Rlp.TxDecoder`1.DecodeLegacyPayloadWithoutSig(T transaction, RlpStream rlpStream) in /src/Nethermind/Nethermind.Serialization.Rlp/TxDecoder.cs:line 104
   at Nethermind.Serialization.Rlp.TxDecoder`1.Decode(RlpStream rlpStream, RlpBehaviors rlpBehaviors) in /src/Nethermind/Nethermind.Serialization.Rlp/TxDecoder.cs:line 74
   at Nethermind.Serialization.Rlp.Rlp.DecodeArray[T](RlpStream rlpStream, IRlpStreamDecoder`1 rlpDecoder, RlpBehaviors rlpBehaviors) in /src/Nethermind/Nethermind.Serialization.Rlp/Rlp.cs:line 155
   at Nethermind.Serialization.Rlp.Rlp.DecodeArray[T](RlpStream rlpStream, RlpBehaviors rlpBehaviors) in /src/Nethermind/Nethermind.Serialization.Rlp/Rlp.cs:line 143
   at Nethermind.Network.P2P.Subprotocols.Eth.V62.Messages.TransactionsMessageSerializer.DeserializeTxs(RlpStream rlpStream) in /src/Nethermind/Nethermind.Network/P2P/Subprotocols/Eth/V62/Messages/TransactionsMessageSerializer.cs:line 60
   at Nethermind.Network.P2P.Subprotocols.Eth.V62.Messages.TransactionsMessageSerializer.Deserialize(IByteBuffer byteBuffer) in /src/Nethermind/Nethermind.Network/P2P/Subprotocols/Eth/V62/Messages/TransactionsMessageSerializer.cs:line 43
   at Nethermind.Network.MessageSerializationService.Deserialize[T](IByteBuffer buffer) in /src/Nethermind/Nethermind.Network/MessageSerializationService.cs:line 46
   at Nethermind.Network.P2P.ProtocolHandlers.ProtocolHandlerBase.Deserialize[T](IByteBuffer data) in /src/Nethermind/Nethermind.Network/P2P/ProtocolHandlers/ProtocolHandlerBase.cs:line 75
   at Nethermind.Network.P2P.Subprotocols.Eth.V62.Eth62ProtocolHandler.HandleMessage(ZeroPacket message) in /src/Nethermind/Nethermind.Network/P2P/Subprotocols/Eth/V62/Eth62ProtocolHandler.cs:line 166
   at Nethermind.Network.P2P.Subprotocols.Eth.V63.Eth63ProtocolHandler.HandleMessage(ZeroPacket message) in /src/Nethermind/Nethermind.Network/P2P/Subprotocols/Eth/V63/Eth63ProtocolHandler.cs:line 59
   at Nethermind.Network.P2P.Subprotocols.Eth.V65.Eth65ProtocolHandler.HandleMessage(ZeroPacket message) in /src/Nethermind/Nethermind.Network/P2P/Subprotocols/Eth/V65/Eth65ProtocolHandler.cs:line 63
   at Nethermind.Network.P2P.Subprotocols.Eth.V66.Eth66ProtocolHandler.HandleMessage(ZeroPacket message) in /src/Nethermind/Nethermind.Network/P2P/Subprotocols/Eth/V66/Eth66ProtocolHandler.cs:line 143
   at Nethermind.Network.P2P.Subprotocols.Eth.V67.Eth67ProtocolHandler.HandleMessage(ZeroPacket message) in /src/Nethermind/Nethermind.Network/P2P/Subprotocols/Eth/V67/Eth67ProtocolHandler.cs:line 61
   at Nethermind.Network.P2P.Subprotocols.Eth.V68.Eth68ProtocolHandler.HandleMessage(ZeroPacket message) in /src/Nethermind/Nethermind.Network/P2P/Subprotocols/Eth/V68/Eth68ProtocolHandler.cs:line 70
   at Nethermind.Network.P2P.Session.ReceiveMessage(ZeroPacket zeroPacket) in /src/Nethermind/Nethermind.Network/P2P/Session.cs:line 213
   at Nethermind.Network.P2P.ProtocolHandlers.ZeroNettyP2PHandler.ChannelRead0(IChannelHandlerContext ctx, ZeroPacket input) in /src/Nethermind/Nethermind.Network/P2P/ProtocolHandlers/ZeroNettyP2PHandler.cs:line 111
   at DotNetty.Transport.Channels.SimpleChannelInboundHandler`1.ChannelRead(IChannelHandlerContext ctx, Object msg)
   at DotNetty.Transport.Channels.AbstractChannelHandlerContext.InvokeChannelRead(Object msg)

marcindsobczak

TestTransaction is fixed. We are still sometimes failing TestLargeTxRequest (which requires us to send 2000 txs), but it is because of timeout in sending block (https://github.com/ethereum/go-ethereum/blob/master/cmd/devp2p/internal/ethtest/suite.go#L458) so doesn't look connected with this PR - looks like we had this issue before and didn't noticed. On the other hand, it would be good to change the moment of calculating tx sizes to limit CPU usage - more details in comment to code

src/Nethermind/Nethermind.Network/P2P/Subprotocols/Eth/V68/Eth68ProtocolHandler.cs

marcindsobczak · 2022-11-17T13:02:01Z

One more thing - we will need to change our behavior in sending full txs. Right now we are limiting it to 256 txs (https://github.com/NethermindEth/nethermind/blob/master/src/Nethermind/Nethermind.Network/P2P/ProtocolHandlers/SyncPeerProtocolHandlerBase.cs#L200), but after introducing new type of txs it might be too much. After merging this PR we will have calculated tx sizes, so will be able to do it same as Geth - limit by data size instead of tx count. But this is definitely another task, I will create issue

# Conflicts: # src/Nethermind/Nethermind.Core/Transaction.cs # src/Nethermind/Nethermind.Network.Test/Builders/SerializationBuilder.cs # src/Nethermind/Nethermind.Network/Metrics.cs # src/Nethermind/Nethermind.Network/P2P/Subprotocols/Eth/V65/IPooledTxsRequestor.cs # src/Nethermind/Nethermind.Network/P2P/Subprotocols/Eth/V65/PooledTxsRequestor.cs # src/Nethermind/Nethermind.Serialization.Rlp/TxDecoder.cs

This reverts commit 77701f6.

This reverts commit 648e156.

src/Nethermind/Nethermind.Network/Metrics.cs

...d.Network/P2P/Subprotocols/Eth/V68/Messages/NewPooledTransactionHashesMessageSerializer68.cs

LukaszRozmej · 2022-12-01T14:04:37Z

src/Nethermind/Nethermind.Network.Test/P2P/Subprotocols/Eth/V68/Eth68ProtocolHandlerTests.cs

+        _handler.ProtocolCode.Should().Be("eth");
+        _handler.Name.Should().Be("eth68");
+        _handler.ProtocolVersion.Should().Be(68);
+        _handler.MessageIdSpaceSize.Should().Be(17);
+        _handler.IncludeInTxPool.Should().BeTrue();
+        _handler.ClientId.Should().Be(_session.Node?.ClientId);
+        _handler.HeadHash.Should().BeNull();
+        _handler.HeadNumber.Should().Be(0);


Why not do the whole comparision:

_handler.Should().BeEquivalentTo(expectedHandler)?

This test is an updated copy-paste of metadata tests for older protocols. Purpose of this test is to check if metadata are correct and it is doing it perfectly. If we create another instance o Eth68ProtocolHandler and compare equivalence with _handler, we will still don't know if these details are correct in both or wrong in both of them. It is not overwritable from the outside, so no way to ensure correctness of expectedHandler

...nd.Network.Test/P2P/Subprotocols/Eth/V68/NewPooledTransactionHashesMessageSerializerTests.cs

src/Nethermind/Nethermind.Network/P2P/Subprotocols/Eth/V65/PooledTxsRequestor.cs

LukaszRozmej · 2022-12-01T14:13:50Z

...d.Network/P2P/Subprotocols/Eth/V68/Messages/NewPooledTransactionHashesMessageSerializer68.cs

+
+        public void Serialize(IByteBuffer byteBuffer, NewPooledTransactionHashesMessage68 message)
+        {
+            byte[] types = message.Types.Select(v => (byte)v).ToArray();


Ok ToArray is not needed if you add method overloads with IReadOnlyList, can we do that?

* eth68 implementation * test * refactor serializer * change typesSize back * Tests and enable capability * serializer tests * Fix whitespaces * fix whitespaces * Refactor * Fix serialization & peer drop * RequestTransactions68 * Metrics & logs * Reduce paket max size * Calculate length properly * fix tests & length * huge transaction test * fix spaces * Make size calculated once per transaction * Refactor & optimizations * Fix hive tests * Copyright fix * ubuntu 20.04 workflow * Revert "ubuntu 20.04 workflow" This reverts commit 77701f6. * Revert "Revert "ubuntu 20.04 workflow"" This reverts commit 648e156. * Remove metrics & for loop * Remove test & RequestTransactionsEth68 * EncodeList implementation * Fix test * Revert renaming * Rename write * Remove Request68 * Fix run-nethermind-tests.yml * Fix run-nethermind-tests.yml Co-authored-by: Marcin Sobczak <marcindsobczak@gmail.com>

deffrian and others added 6 commits November 3, 2022 12:15

eth68 implementation

0299694

test

b734ab5

refactor serializer

692ef07

change typesSize back

e8b67ee

Tests and enable capability

c8059ad

serializer tests

f5feb4e

deffrian linked an issue Nov 5, 2022 that may be closed by this pull request

Implement ETH/68 (EIP-5793) #4821

Closed

deffrian added 2 commits November 6, 2022 16:37

Fix whitespaces

a7560b1

fix whitespaces

d7097d9

marcindsobczak reviewed Nov 7, 2022

View reviewed changes

deffrian added 4 commits November 9, 2022 12:20

Merge branch 'master' into feature/eth68

a0ccb0b

Refactor

cfbc257

Fix serialization & peer drop

260d7ee

RequestTransactions68

f8e1f33

deffrian marked this pull request as ready for review November 11, 2022 09:41

deffrian requested review from marcindsobczak and MarekM25 November 11, 2022 09:41

Metrics & logs

ba8f74d

marcindsobczak requested a review from LukaszRozmej November 11, 2022 12:16

marcindsobczak requested changes Nov 11, 2022

View reviewed changes

deffrian added 5 commits November 12, 2022 12:16

Reduce paket max size

f0b3442

Calculate length properly

5a10857

fix tests & length

a2a1f2f

huge transaction test

b892322

fix spaces

5881a23

marcindsobczak reviewed Nov 17, 2022

View reviewed changes

src/Nethermind/Nethermind.Network/P2P/Subprotocols/Eth/V68/Eth68ProtocolHandler.cs Outdated Show resolved Hide resolved

deffrian added 8 commits November 20, 2022 21:08

Refactor & optimizations

e123c19

Fix hive tests

e84f311

Copyright fix

81f6d18

ubuntu 20.04 workflow

77701f6

Merge branch 'master' into feature/eth68

3a15b7f

Revert "ubuntu 20.04 workflow"

648e156

This reverts commit 77701f6.

Revert "Revert "ubuntu 20.04 workflow""

89991e8

This reverts commit 648e156.

asdacap reviewed Nov 29, 2022

View reviewed changes

src/Nethermind/Nethermind.Network/Metrics.cs Outdated Show resolved Hide resolved

asdacap reviewed Nov 29, 2022

View reviewed changes

...d.Network/P2P/Subprotocols/Eth/V68/Messages/NewPooledTransactionHashesMessageSerializer68.cs Outdated Show resolved Hide resolved

asdacap approved these changes Nov 29, 2022

View reviewed changes

Remove metrics & for loop

ae32d99

flcl42 mentioned this pull request Nov 29, 2022

EIP-4844: Shard Blob Transactions #4558

Closed

LukaszRozmej reviewed Dec 1, 2022

View reviewed changes

deffrian added 6 commits December 1, 2022 20:50

Remove test & RequestTransactionsEth68

a82d574

EncodeList implementation

63ddc23

Fix test

b04541f

Revert renaming

dc84be6

Rename write

9227f4f

Remove Request68

879e1a7

marcindsobczak mentioned this pull request Dec 6, 2022

Refactor TxBroadcaster to not broadcast full blob txs #4968

Closed

LukaszRozmej approved these changes Dec 13, 2022

View reviewed changes

deffrian added 3 commits December 14, 2022 12:43

Fix run-nethermind-tests.yml

7793937

Fix run-nethermind-tests.yml

ac14f58

Merge branch 'master' into feature/eth68

7ae7424

deffrian merged commit 120c623 into master Dec 15, 2022

deffrian deleted the feature/eth68 branch December 15, 2022 14:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for eth68 #4866

Add support for eth68 #4866

deffrian commented Nov 5, 2022

marcindsobczak left a comment

marcindsobczak commented Nov 11, 2022

deffrian commented Nov 11, 2022

marcindsobczak left a comment

marcindsobczak commented Nov 11, 2022

marcindsobczak left a comment

marcindsobczak commented Nov 17, 2022

LukaszRozmej Dec 1, 2022

marcindsobczak Dec 2, 2022

LukaszRozmej Dec 1, 2022

Add support for eth68 #4866

Add support for eth68 #4866

Conversation

deffrian commented Nov 5, 2022

Changes:

Types of changes

Testing

marcindsobczak left a comment

Choose a reason for hiding this comment

marcindsobczak commented Nov 11, 2022

deffrian commented Nov 11, 2022

marcindsobczak left a comment

Choose a reason for hiding this comment

marcindsobczak commented Nov 11, 2022

marcindsobczak left a comment

Choose a reason for hiding this comment

marcindsobczak commented Nov 17, 2022

LukaszRozmej Dec 1, 2022

Choose a reason for hiding this comment

marcindsobczak Dec 2, 2022

Choose a reason for hiding this comment

LukaszRozmej Dec 1, 2022

Choose a reason for hiding this comment